Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Data Import ux redo #1754

Merged
merged 96 commits into from
Dec 22, 2023
Merged

Data Import ux redo #1754

merged 96 commits into from
Dec 22, 2023

Conversation

mallexxx
Copy link
Collaborator

@mallexxx mallexxx commented Oct 12, 2023 •
edited
Loading

Task/Issue URL: https://app.asana.com/0/0/1204180319229906/f

Description:

  • Refactored import with SwiftUI
  • Added (preparation for) progress reporting - always showing indefinite for now
  • Added manual import step for failed imports and instructions
  • Added feedback form on import failure
  • Converted localization to Localizable.xcstrings

Steps to test this PR:

  • Validate import flows according to the figma: https://www.figma.com/file/1vcDmnUd3LXsKyDIMpJDid/branch/ARZJgjRzbYg1ENhabvT0hi/Untitled?type=design&node-id=0-1&mode=design&t=b6pXfREI2WURt7PL-0
  • Backup data and corrupt browser profiles: when bookmarks and/or passwords files missing - the browser profile should still be marked for import - but csv/html import instructions displayed
  • for a corrupted browser profile - when import of bookmarks and/or passwords fails - file import instructions should be displayed.
  • If import still fails (e.g. for html/csv corrupted files) - send feedback screen should be displayed, feedback should be added to assana with appropriate feedback tag
  • Validate import during onboarding and setting according user defaults setting is correct
  • Validate notification sent on import launches sync as expected
Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

mallexxx added 15 commits October 9, 2023 15:31
@mallexxx
Add new browser import sources
988dbc8
@mallexxx
fix tests
5cdfa77
@mallexxx
Bitwarden source, Refactor CSV parser, improve column detection
b1ee94a
@mallexxx
minor cleanup
a64117c
@mallexxx
cleanup
8521ae3
@mallexxx
Add Passwords import as CSV step to Yandex
24a24ac
@mallexxx
rename ChromiumPreferences
4d21895
@mallexxx
handle ImportError separately in DataImportViewController
b60be62
@mallexxx
rename ChromiumPreferences in pbxproj
748c285
@mallexxx
fix browser isInstalled detection
e7a5566
@mallexxx
Fix a glitch on switching import button state when changing import so…
fc03dd1 
…urces; Update Firefox MoreInfo to include actual browser name
@mallexxx
Merge remote-tracking branch 'origin/develop' into alex/new-import-br…
bafd5e7 
…owsers
@mallexxx
Merge branch 'alex/new-import-browsers' into alex/improve-import-info
620a01e
@mallexxx
Merge branch 'alex/new-import-csv' into alex/improve-import-info
46de2a9
@mallexxx
Disable import passwords/bookmarks checkboxes if data file not found
491a801
@mallexxx mallexxx requested a review from ayoy October 12, 2023 09:24
@mallexxx mallexxx self-assigned this Oct 12, 2023
@mallexxx mallexxx changed the title Disables data import checkboxes if data file not found [WIP] Disables data import checkboxes if data file not found Oct 12, 2023
@mallexxx
Merge remote-tracking branch 'origin/develop' into alex/improve-impor…
8f4531c 
…t-info
@mallexxx
Merge branch 'develop' into alex/new-import-browsers
c642307
@mallexxx
Bitwarden source, Refactor CSV parser, improve column detection (#1740)
c50b7f0 
Task/Issue URL:
https://app.asana.com/0/1199178362774117/1202408404935489/f

**Description**:
- Added Bitwarden CSV Import Source
- Refactored CSV parser: 
- parsing was incorrectly handling quote characters escaped by backslash
but should be double quotes instead
  - now passwords with \ and " should be imported correctly
- Improved CSV headers detection to support Bitwarden, Zoho (general +
vault formats), RoboForm, Dashlane

**Steps to test this PR**:
1. Validate CSV import from password managers stated above as well as
existing CSV integrations (1Password, LastPass, Safari)

<!--
Tagging instructions
If this PR isn't ready to be merged for whatever reason it should be
marked with the `DO NOT MERGE` label (particularly if it's a draft)
If it's pending Product Review/PFR, please add the `Pending Product
Review` label.

If at any point it isn't actively being worked on/ready for
review/otherwise moving forward (besides the above PR/PFR exception)
strongly consider closing it (or not opening it in the first place). If
you decide not to close it, make sure it's labelled to make it clear the
PRs state and comment with more information.
-->

---
###### Internal references:
[Pull Request Review
Checklist](https://app.asana.com/0/1202500774821704/1203764234894239/f)
[Software Engineering
Expectations](https://app.asana.com/0/59792373528535/199064865822552)
[Technical Design
Template](https://app.asana.com/0/59792373528535/184709971311943)
[Pull Request
Documentation](https://app.asana.com/0/1202500774821704/1204012835277482/f)
@mallexxx
Merge remote-tracking branch 'origin/develop' into alex/improve-impor…
1358f18 
…t-info
@mallexxx mallexxx changed the base branch from develop to alex/new-import-browsers November 17, 2023 10:35
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 17, 2023 •
edited
Loading

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS against ec97729

@mallexxx mallexxx changed the title [WIP] Disables data import checkboxes if data file not found [WIP] Data Import ux redo Nov 17, 2023
@samsymons samsymons self-requested a review December 21, 2023 02:16
@samsymons
Copy link
Collaborator

I noticed that the text area in the import failed state looks a bit strange in dark mode. If it's intentional, please ignore this.

Import Failed State

@samsymons
Copy link
Collaborator

samsymons commented Dec 21, 2023 •
edited
Loading

I had tested this previously and it seemed to be fine, but tonight I'm unable to import passwords from Firefox at all - initially I had a primary password set, but I've removed that and it still failed.

The error is FirefoxLoginReader.ImportError.OperationType.couldNotDetermineFormat, which appears to be because the login reader can't find the logins.json file. I checked my Firefox data directory to see the following profiles:

  • 5eaiov1m.default-release-1686882746641
  • 08qtydft.default
  • uu7meg9z.default-release

The login reader is picking the last option, uu7meg9z.default-release, but this directory is completely empty for me - it's the first one that has all my data.

My guess is that it's this 1686882746641 suffix (looks like a timestamp?) on the profile that's causing problems here.

Note that this works in 1.69.0, so it appears to be a regression.

samsymons
samsymons reviewed Dec 21, 2023
View reviewed changes
DuckDuckGo/DataImport/Model/InstructionsFormatParser.swift
import Foundation

/// NSLocalizedString format parser for CSV/HTML data import instructions screen
struct InstructionsFormatParser {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍

@mallexxx
Copy link
Collaborator Author

mallexxx commented Dec 21, 2023 •
edited
Loading

Thanks, @sam, great catch!
Should be fixed now: updated default profile selection to choose from validated profiles first
Refactored EditableTextView to show focus ring

@samsymons
Copy link
Collaborator

Thanks! I'll re-test this within 30 minutes.

@samsymons samsymons self-requested a review December 21, 2023 16:41
samsymons
samsymons approved these changes Dec 21, 2023
View reviewed changes
Copy link
Collaborator

@samsymons samsymons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new changes look great, thanks! Tested all import options one final time, all appears to work well. This is a really nice change, awesome work on this.

@mallexxx mallexxx mentioned this pull request Dec 22, 2023
Closed
@mallexxx mallexxx merged commit ae8a4e0 into main Dec 22, 2023
14 checks passed
@mallexxx mallexxx deleted the alex/improve-import-info branch December 22, 2023 07:17
samsymons added a commit that referenced this pull request Dec 24, 2023
@samsymons
Merge branch 'main' into sam/add-netp-subscription-auth-support
52f025c 
# By Diego Rey Mendez (4) and others
# Via Fernando Bunn (2) and GitHub (1)
* main:
  Adds option + click support for our VPN menu to show some useful debu… (#2007)
  Update latency & tunnel failure monitor implementation (#2005)
  Prevents VPNSettings from reporting fake changes (#2004)
  Updates the copy for the VPN status bar item context menu (#2003)
  Implement subscription purchase (#1906)
  DBP: Add m_mac prefix to Pixels (#1952)
  Bump version to 1.69.0 (96)
  Update embedded files
  Update Link Tracking Protection to preserve headers (#1965)
  VPN menus improvements (#1979)
  Data Import ux redo (#1754)
  Fix: "SwiftLintPlugin" must be enabled before it can be used (#1987)

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
#	DuckDuckGo/NavigationBar/View/NavigationBarViewController.swift
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ayoy ayoy ayoy left review comments

@samsymons samsymons samsymons approved these changes

Assignees

@mallexxx mallexxx

Labels
pending ship review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mallexxx @samsymons @ayoy